Skip to content

update to fix data race - #39

Open
IrisesD wants to merge 1 commit into
patrikeh:masterfrom
IrisesD:master
Open

update to fix data race#39
IrisesD wants to merge 1 commit into
patrikeh:masterfrom
IrisesD:master

Conversation

@IrisesD

@IrisesD IrisesD commented Jan 16, 2024

Copy link
Copy Markdown

For now, the Shuffle() function in training/model.go is:

func (e Examples) Shuffle() {
	for i := range e {
		j := rand.Intn(i + 1)
		e[i], e[j] = e[j], e[i]
	}
}

which uses the rand.Intn and performs write in a concurrent way. When the Shuffle() function is called in a concurrent way, there will be race condition error:

WARNING: DATA RACE
Read at 0x00c0001280c0 by goroutine 25:
  github.com/patrikeh/go-deep/training.Examples.Shuffle()
      /Users/xxx/go/pkg/mod/github.com/patrikeh/go-deep@v0.0.0-20230427173908-a2775168ab3d/training/model.go:18 +0xd5
  github.com/patrikeh/go-deep/training.(*OnlineTrainer).Train()
      /Users/xxx/go/pkg/mod/github.com/patrikeh/go-deep@v0.0.0-20230427173908-a2775168ab3d/training/trainer.go:54 +0x40d
  test.BenchmarkShuffle.func3()
      /Users/xxx/Desktop/Train.go:51 +0xba

Previous write at 0x00c0001280c0 by goroutine 24:
  github.com/patrikeh/go-deep/training.Examples.Shuffle()
      /Users/xxx/go/pkg/mod/github.com/patrikeh/go-deep@v0.0.0-20230427173908-a2775168ab3d/training/model.go:18 +0x1b9
  github.com/patrikeh/go-deep/training.(*OnlineTrainer).Train()
      /Users/xxx/go/pkg/mod/github.com/patrikeh/go-deep@v0.0.0-20230427173908-a2775168ab3d/training/trainer.go:54 +0x40d
  test.BenchmarkShuffle.func2()
      /Users/xxx/Desktop/Train.go:50 +0xba

I suggest we should add a mutex lock to the function, as this PR does.

@IrisesD IrisesD closed this Jan 17, 2024
@IrisesD IrisesD reopened this Jan 17, 2024
@incase

incase commented May 16, 2026

Copy link
Copy Markdown

There is no concurrency here (this is a documented compiler ability). And if there was, the mutex wouldn't do anything, since the only concurrency is in the assignment itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants